-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
#5947 - roll up bulk asset checkout email #18095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
#5947 - roll up bulk asset checkout email #18095
Conversation
|
@snipe you rightfully mentioned that checking out items from different categories with long EULAs might trigger some truncation or character limit. Can you look at my overall approach in this PR and let me know if it is worth continuing down this path? If so, I'm tentatively thinking I can account for this by limiting the number of categories being sent per email and potentially including logic around EULA length. |
|
That's an interesting approach. Breaking them out by their EULAs, so each unique EULA gets an email with one or more items in it.... I think I like that. |
|
Moving this to a draft while I hash out the potential truncation or character limit issue. |
resources/views/mail/markdown/bulk-asset-checkout-mail.blade.php
Outdated
Show resolved
Hide resolved
|
I deleted the comments from the dev meeting because I think they're ones we can address when they come up. There are for sure some things we'll still need to address, but I think this covers the majority of the use-cases. Nothing is perfect, and as we discussed on the dev meeting, some folks use us in really unusual ways - but I think we can tweak that moving forward as those scenarios arise. |
|
So here's my example, and my proposed solution (which @snipe said she hates, but, well, I should at least write it down). Let's imagine our categories and EULA's and stuff look like this -
The thing is, with the current EULA structure, we're going to display and re-display the same EULAs to people a couple of times with our category-centric ways of handling this. So my idea was to normalize the EULAs into their own table. And then you can select which EULA you want for which category (or NULL for 'use default'? Or is that the use-default checkbox?) With this normalized EULA, we get a few things at the same time - we might be able to store versions of a EULA, to know which one was signed? And we can 'roll up' lists of assets that are checked out to say |
|
@snipe Yeah, I agree. I think we'll be okay with merging this as is and then figure things out if (as) they bubble up. Something to consider in the future is moving the logic that groups assets by category from the mailable to the listener so you have a hook to say "is this email too long? let's fire off an email per category instead" or something similar. |
I mean "merging as is" after a review of course 😅 |
Currently, checking out multiple assets via the bulk checkout feature would result in individual emails being sent for each asset that was included in the checkout. This PR changes that behavior by sending one email that contains all assets that were checked out.
I attempted to make the resulting email as readable as possible so the new
BulkAssetCheckoutMailitself does a lot of work to "bundle" up groups of assets.A few examples
Assets not requiring acceptance:

"Bulk" checking out a single asset:

Checking out assets where some items require acceptance:

Technical
Here are some notes on how this is technically implemented:
CheckoutablesCheckedOutInBulk, and listener for that event.CheckoutableListenerbut I separated it out since we're now dealing with an array of items and I didn't want to add a bunch of conditionals into the existing class.bulk_asset_checkoutto the request's context in the controller layer and checking for that context in the existingCheckoutableListenerto avoid sending individual notifications.CheckoutableListenermore complex than it already is.Additional notes
Partially addresses #5947 (sends one email for the entire bulk checkout instead of individual emails for each asset). If acceptance is required the user will still need to accept each asset individually.